Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a UI4 modal #4642

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Add a UI4 modal #4642

merged 3 commits into from
Jan 12, 2024

Conversation

swansontec
Copy link
Contributor

@swansontec swansontec commented Dec 19, 2023

image

New changes (with one updated modal per new built-in "padding"):
image

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

bridge: AirshipBridge<T>
children?: React.ReactNode

// Called when the user taps outside the modal or clicks the back button:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Called when the user taps outside the modal or clicks the back button:
// Called when the user taps outside the modal, clicks the back button, or drags the modal down:

borderTopLeftRadius: theme.rem(1),
borderTopRightRadius: theme.rem(1),
flexShrink: 1,
width: theme.rem(30) // This works as a maxWidth because flexShrink is set
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this good for tablets?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Comment is out of scope for this review. We have an upcoming discussion next week regarding tablet layouts

@Jon-edge
Copy link
Collaborator

Jon-edge commented Jan 3, 2024

Would like to see in this PR a blanket replacement of the original modals, shouldn't be any issues in doing this?

const theme = useTheme()
const styles = getStyles(theme)

const padding = sidesToPadding(mapSides(fixSides(paddingRem, 1), theme.rem))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In UI3, we had scenes with (mostly) 0rem padding, and modals (I think mostly if not all) had 1rem padding. This made component reuse tricky in UI3 between scenes and modals.

UI4 components all expect 0.5rem margins at a minimum on their modal/scene parents and neighbors/siblings. Specifically margins instead of padding, because using margins allows for children to set negative margins for the few exceptions where we need full widths:

  1. Full width dividers (trying to stop using these in the future according to Madison, whitespace is key to cleanliness)
  2. Carousels

Let me know if there's something super wrong about the above.

warning?: boolean

// Called when the user taps outside the modal or clicks the back button.
// If this is missing, the modal will not be closable.
Copy link
Collaborator

@Jon-edge Jon-edge Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this is for modals that can only be dismissed after user checks an
"I understand" checkbox or similar. I found this pattern inconvenient in UI3.

Since this is the exception, not the rule, suggest we make the default behavior
taken care of by the modal (gracefully closing) without extra empty onCancel code from the
caller, and have a special optional prop lockUntilAction, persistent, nonExitable, completionRequired, exitRestricted, etc that blocks this default behavior.

Only need to update the few callers that have this special case, and leave the
rest as-is even though onCancel is no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to make the modal self-dismiss, since the modal doesn't have access to the Airship promise. You must handle the onCancel callback by doing a bridge.resolve, or the promise will be left dangling forever. This change simply aligns the visual with this existing reality.

borderTopLeftRadius: theme.rem(1),
borderTopRightRadius: theme.rem(1),
flexShrink: 1,
width: theme.rem(30) // This works as a maxWidth because flexShrink is set
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Comment is out of scope for this review. We have an upcoming discussion next week regarding tablet layouts

@@ -550,6 +550,8 @@ export const edgeDark: Theme = {
start: { x: 1, y: 0 }
},

modalBackgroundUi4: '#00000060',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for palette colors? I'm fine either way, would love to avoid the extra boilerplate if something isn't reused.

top: 0
},
dragBar: {
// TODO: Replace this color we have a proper design:
Copy link
Collaborator

@Jon-edge Jon-edge Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shot a mention to Madison, we might as well get it all in before merge to avoid another change

Copy link
Collaborator

@Jon-edge Jon-edge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few changes, but it feels good to use. Would be nice to get the theme/styling figured out before merge unless there's a real rush for staging

@Jon-edge Jon-edge force-pushed the william/ui4-modal branch 2 times, most recently from 40772b7 to 258cc65 Compare January 12, 2024 03:43
@paullinator paullinator disabled auto-merge January 12, 2024 08:56
@paullinator paullinator merged commit 95d17ec into develop Jan 12, 2024
2 checks passed
@paullinator paullinator deleted the william/ui4-modal branch January 12, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants